-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MG-2080 - Add TLS Certs Loading and Certificate Revocation #57
Conversation
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
examples/client/http/with_mtls.sh
Outdated
curl -sSiX POST "${protocol}://${host}:${port}/${path}" -H "content-type:${content}" -H "Authorization:TOKEN" -d "${message}" --cacert $cafile 2>&1 | ||
|
||
echo -e "\nPosting message to ${protocol}://${host}:${port}/${path} with tls, Authorization header, & without ca , client certificates.." | ||
curl -sSiX POST "${protocol}://${host}:${port}/${path}" -H "content-type:${content}" -H "Authorization:TOKEN" -d "${message}" 2>&1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update Readme with details on how to use examples and create relevant certificates using Makefile.
config.go
Outdated
} | ||
|
||
func (c *Config) EnvParse(opts env.Options) error { | ||
if len(opts.FuncMap) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use switch instead of if-else
.
pkg/tls/tls.go
Outdated
ClientCertValidationMethods []ValidateMethod `env:"CLIENT_CERT_VALIDATION_METHODS" envDefault:""` | ||
OCSPDepth uint `env:"OCSP_DEPTH" envDefault:"0"` | ||
OCSPResponderURL url.URL `env:"OCSP_RESPONDER_URL" envDefault:""` | ||
CRLDepth uint `env:"CRL_DEPTH" envDefault:"1"` | ||
OfflineCRLFile string `env:"OFFLINE_CRL_FILE" envDefault:""` | ||
CRLDistributionPoints url.URL `env:"CRL_DISTRIBUTION_POINTS" envDefault:""` | ||
CRLDistributionPointsSignCheck bool `env:"CRL_DISTRIBUTION_POINTS_SIGN_CHECK" envDefault:"false"` | ||
CRLDistributionPointsIssuerCertFile string `env:"CRL_DISTRIBUTION_POINTS_ISSUER_CERT_FILE " envDefault:""` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract these (revocation config) to a separate struct in a separate file.
pkg/tls/tls.go
Outdated
} | ||
|
||
// Load return a TLS configuration that can be used in TLS servers. | ||
func (c *Config) Load() (*tls.Config, Security, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not return Security
here. Returning nil, nil
to indicate no config and no error seems a little cryptic (while correct, so I would suggest using the ErrNoTLS
error to indicate no error, but also no TLS; so you can later have something like:
l, err := net.Listen("tcp", p.config.Address)
if err != nil {
return err
}
tlsCfg, err := p.config.TLSConfig.Load()
switch err {
case nil:
l = tls.NewListener(l, tlsCfg)
case mptls.ErrNoTLS:
break
default:
return err
}
While this means we first start the listener and then parse the TLS config, this order is not so important; we can't have one without the other anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to know config is mTLS or only TLS , How can we find that config is TLS or mTLS ?
pkg/tls/tls.go
Outdated
return []byte{}, nil | ||
} | ||
|
||
func (c *Config) verifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this alongside the new file for revocation config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this method looks very complex. Can you try to simplify or, at least, split?
pkg/tls/tls.go
Outdated
if !isRootCA(peerCertificate) { | ||
if issuerCert == nil { | ||
if len(peerCertificate.IssuingCertificateURL) < 1 { | ||
return fmt.Errorf("neither the issuer certificate is present in the chain nor is the issuer certificate URL present in AIA, certificate common name %s and serial number %x", peerCertificate.Subject.CommonName, peerCertificate.SerialNumber) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract large strings to constants. Try to simplify the method.
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace if-else
with switch
.
pkg/tls/tls.go
Outdated
type Security int | ||
|
||
const ( | ||
WithoutTLS Security = iota + 1 | ||
WithTLS | ||
WithmTLS | ||
WithmTLSVerify | ||
) | ||
|
||
func (s Security) String() string { | ||
switch s { | ||
case WithTLS: | ||
return "with TLS" | ||
case WithmTLS: | ||
return "with mTLS" | ||
case WithmTLSVerify: | ||
return "with mTLS and validation of client certificate revocation status" | ||
case WithoutTLS: | ||
fallthrough | ||
default: | ||
return "without TLS" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this whole construct and replace it with:
func Status(c Config) string {
if c.CertFile == "" && c.KeyFile == "" {
return "TLS"
}
if c.ClientCAFile != "" {
if len(c.ClientValidation.ValidationMethods) > 0 {
return "mTLS with client verification"
}
return "mTLS"
}
return "no TLS"
}
In main.go
we have:
p.logger.Info(fmt.Sprintf("HTTP proxy server at %s%s with %s exiting with errors", p.config.Address, p.config.PrefixPath, mptls.Status(p.config.TLSConfig)), slog.String("error", err.Error()))
pkg/tls/validations.go
Outdated
@@ -0,0 +1,434 @@ | |||
// Copyright (c) Abstract Machines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to verifier
. Also, separate OCSP and CRL into subpackages.
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
config.go
Outdated
func (c *Config) EnvParse(opts env.Options) error { | ||
switch { | ||
case len(opts.FuncMap) == 0: | ||
opts.FuncMap = map[reflect.Type]env.ParserFunc{ | ||
reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods): parseSliceValidateMethod, | ||
reflect.TypeOf(verifier.OCSP): parseValidateMethod, | ||
} | ||
default: | ||
opts.FuncMap[reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods)] = parseSliceValidateMethod | ||
opts.FuncMap[reflect.TypeOf(verifier.OCSP)] = parseValidateMethod | ||
} | ||
return env.ParseWithOptions(c, opts) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify:
func (c *Config) EnvParse(opts env.Options) error { | |
switch { | |
case len(opts.FuncMap) == 0: | |
opts.FuncMap = map[reflect.Type]env.ParserFunc{ | |
reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods): parseSliceValidateMethod, | |
reflect.TypeOf(verifier.OCSP): parseValidateMethod, | |
} | |
default: | |
opts.FuncMap[reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods)] = parseSliceValidateMethod | |
opts.FuncMap[reflect.TypeOf(verifier.OCSP)] = parseValidateMethod | |
} | |
return env.ParseWithOptions(c, opts) | |
} | |
func (c *Config) EnvParse(opts env.Options) error { | |
if opts.FuncMap == nil { | |
opts.FuncMap = make(map[reflect.Type]env.ParserFunc) | |
} | |
opts.FuncMap[reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods)] = parseSliceValidateMethod | |
opts.FuncMap[reflect.TypeOf(verifier.OCSP)] = parseValidateMethod | |
return env.ParseWithOptions(c, opts) | |
} |
pkg/tls/tls.go
Outdated
return tlsConfig, nil | ||
} | ||
|
||
func (c *Config) Security() Security { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this as a util function, do not use Config receiver. Also, move it to a different package.
pkg/tls/tls.go
Outdated
Verifier verifier.Config | ||
} | ||
|
||
func (c *Config) SecurityStatus() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, remove; use util method.
pkg/tls/tls.go
Outdated
// LoadTLSCfg return a TLS configuration that can be used in TLS servers. | ||
func LoadTLSCfg(ca, crt, key string) (*tls.Config, error) { | ||
caCertPEM, err := os.ReadFile(ca) | ||
func (s Security) String() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove, we do not need a new type; use the util method to determine log levels.
pkg/tls/tls.go
Outdated
KeyFile string `env:"KEY_FILE" envDefault:""` | ||
ServerCAFile string `env:"SERVER_CA_FILE" envDefault:""` | ||
ClientCAFile string `env:"CLIENT_CA_FILE" envDefault:""` | ||
Verifier verifier.Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with the list of verifiers, where Verifier is:
type Verifier interface {
// VerifyPeerCertificate...
VerifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error
}
and in the Config we have:
func (c *Config) VerifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
for _, v := range c.Verifiers {
if err := v.VerifyPeerCertificate(rawCerts, verifiedChains); err != nil {
return err
}
}
return nil
}
And you use different implementations of Verifier ocsp
and crl
just like you already did.
pkg/tls/tls.go
Outdated
} | ||
tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert | ||
if len(c.Verifier.ValidationMethods) > 0 { | ||
tlsConfig.VerifyPeerCertificate = c.Verifier.VerifyPeerCertificate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here it would be something like:
tlsConfig.VerifyPeerCertificate = c.VerifyPeerCertificate
Signed-off-by: Arvindh <[email protected]>
Signed-off-by: Arvindh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix CI liner comments.
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
NOISSUE - Refactor TLS layer
README.md
Outdated
- Script to test mProxy server running at port 1884 for MQTT without TLS | ||
|
||
```bash | ||
examples/client/mqtt/without_tls.sh | ||
``` | ||
|
||
- Script to test mProxy server running at port 8883 for MQTT with TLS | ||
|
||
```bash | ||
examples/client/mqtt/with_tls.sh | ||
``` | ||
|
||
- Script to test mProxy server running at port 8884 for MQTT with mTLS | ||
|
||
```bash | ||
examples/client/mqtt/with_mtls.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix examples/client/http/...
.
Signed-off-by: Arvindh <[email protected]>
@@ -0,0 +1,3 @@ | |||
listener 1883 | |||
listener 8000 | |||
protocol websockets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add allow_anonymous true
to allow using more recent Mosquitto versions.
Signed-off-by: Arvindh <[email protected]>
Pull request title should be
MF-XXX - description
orNOISSUE - description
where XXX is ID of issue that this PR relate to.Please review the CONTRIBUTING.md file for detailed contributing guidelines.
What does this do?
Add TLS Certs Loading and Certificate Revocation
Which issue(s) does this PR fix/relate to?
Resolves https://github.com/absmach/magistrala/issues/2080.
List any changes that modify/break current functionality
Have you included tests for your changes?
Did you document any new/modified functionality?
Notes